Fix pre-existing code quality bugs (#289) by nitrobass24 · Pull Request #312 · nitrobass24/seedsync

nitrobass24 · 2026-03-24T01:29:44Z

Summary

Fixes 7 pre-existing code quality bugs identified during the Ruff review in #289:

Busy-wait CPU spike on shutdown: AppProcess.terminate() had a while self.is_alive(): pass loop burning CPU. Added time.sleep(0.05) to yield the CPU.
Race condition on queue internals: ExtractDispatch accessed self.__task_queue.queue (the raw deque) without holding the queue mutex. Wrapped all accesses in with self.__task_queue.mutex:.
user=None produces "None@host": Sshcp formatted "{}@{}".format(self.__user, self.__host) unconditionally. Added _remote_address() helper that omits user when None.
Wrong boolean in test wait loop: test_extract_ignores_duplicate_calls used and instead of or, causing premature loop exit.
urlopen not closed: WebhookNotifier._send_post() didn't close the response from urlopen. Wrapped in with statement.
Popen not awaited (test_extract): setUpClass used subprocess.Popen without waiting for completion. Replaced with subprocess.run(..., check=True).
fnull not closed (test_controller): _create_archive leaked an open(os.devnull) handle. Replaced with subprocess.run(..., stdout=subprocess.DEVNULL, check=True).

Test plan

All 18 existing test_dispatch.py tests pass
Added 3 new unit tests for Sshcp._remote_address() (user set, user None, default)
ruff format and ruff check pass on all changed files
Full unit test suite passes (550 passed; pre-existing lftp failures unrelated)

Closes #289

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

Bug Fixes
- Fixed race conditions in task queue operations during concurrent access.
- Resolved SSH connection issues when user credentials are not specified.
- Improved HTTP response handling to prevent connection resource leaks.
Performance
- Optimized process termination to use efficient polling instead of busy-waiting, reducing CPU usage.

- Fix busy-wait CPU spike on shutdown in AppProcess.terminate() - Fix race condition on queue internals in ExtractDispatch - Fix user=None producing "None@host" in Sshcp with _remote_address helper - Fix wrong boolean operator in test_dispatch wait loop (and -> or) - Fix unclosed urlopen response in WebhookNotifier - Fix leaked Popen/fnull in integration test setup (use subprocess.run) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

coderabbitai · 2026-03-24T01:30:02Z

Warning

Rate limit exceeded

@nitrobass24 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 19 minutes and 16 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c95c2e06-4a78-47bb-b578-0b32d880710a

📥 Commits

Reviewing files that changed from the base of the PR and between 17cc5e7 and 38d2052.

📒 Files selected for processing (1)

src/python/ssh/sshcp.py

📝 Walkthrough

Walkthrough

This PR addresses multiple pre-existing code quality issues identified during the Ruff linting review (#289). Changes include: replacing a CPU-spinning busy-wait loop with throttled sleep, adding synchronization around queue inspection to prevent race conditions, ensuring HTTP response handles are properly closed via context managers, handling None user values in SSH connections, fixing subprocess resource leaks, and correcting test wait logic.

Changes

Cohort / File(s)	Summary
Concurrency & Synchronization `src/python/common/app_process.py`, `src/python/controller/extract/dispatch.py`	Replaced busy-wait CPU spin loop with `time.sleep(0.05)` throttling in process termination. Added explicit mutex acquisition around queue inspection in dispatch to prevent race conditions when reading/checking queued tasks and peeking at next task.
Resource Management `src/python/controller/notifier.py`, `src/python/tests/integration/test_controller/test_controller.py`, `src/python/tests/integration/test_controller/test_extract/test_extract.py`	Wrapped HTTP POST calls in context manager for proper response handle closure. Replaced `subprocess.Popen` with `subprocess.run()` using `subprocess.DEVNULL` and `check=True` to ensure output suppression and proper error handling without resource leaks.
SSH Path Handling `src/python/ssh/sshcp.py`, `src/python/tests/unittests/test_ssh/test_sshcp_remote_address.py`	Added `_remote_address()` helper method to handle `user=None` case, preventing invalid `None@host` formatting. Updated SSH execution paths to use this helper. Added comprehensive unit tests covering user-set, user-None, and default-user scenarios.
Test Logic `src/python/tests/unittests/test_controller/test_extract/test_dispatch.py`	Corrected spin-wait conditions from `and` to `or` in two test methods (`test_extract_calls_listener_on_completed`, `test_extract_ignores_duplicate_calls`) to fix premature loop termination that could cause incorrect test assertions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Migrate multiprocessing from fork to spawn #232: Modifies the AppProcess class in src/python/common/app_process.py; this PR updates terminate() behavior while the related PR addresses multiprocessing logging/startup behavior in the same class.

Poem

🐰 A rabbit's ode to cleaner code,
where mutexes guard the queue abode,
no more spinning, just gentle sleep,
resources closed, promises we keep,
None becomes host when users depart,
process and stream both play their part! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 35.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix pre-existing code quality bugs (`#289`)' accurately describes the main objective of the PR: addressing multiple pre-existing bugs identified during a Ruff review.
Linked Issues check	✅ Passed	The PR successfully addresses all the real bugs and resource leaks specified in issue `#289`: CPU busy-wait in AppProcess.terminate() fixed with time.sleep(), queue race condition fixed with mutex guards, user=None formatting fixed with _remote_address() helper, boolean logic in test wait loop corrected from 'and' to 'or', urlopen response properly closed with context manager, and subprocess leaks eliminated using subprocess.run() with check=True and DEVNULL.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with the coding requirements in issue `#289`; no out-of-scope modifications are present. Design decisions and deferred items (os.replace permissions, remote path joining, no-enabled-pairs logic) mentioned in the issue were appropriately excluded from this PR.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/ruff-real-bugs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/python/ssh/sshcp.py`:
- Around line 42-46: Sshcp.copy() still builds a remote target with a direct
"{user}@{host}" format which can produce "None@host"; update the Sshcp.copy()
implementation to call the existing helper _remote_address() (the method defined
as def _remote_address(self) -> str) wherever the code constructs the remote
"user@host" string so the logic that returns just host when user is None is
reused and the None@host output is eliminated.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 66afc1e1-8ada-4d64-9e3c-a1d7b5fec68e

📥 Commits

Reviewing files that changed from the base of the PR and between 1fa5dee and 17cc5e7.

📒 Files selected for processing (8)

src/python/common/app_process.py
src/python/controller/extract/dispatch.py
src/python/controller/notifier.py
src/python/ssh/sshcp.py
src/python/tests/integration/test_controller/test_controller.py
src/python/tests/integration/test_controller/test_extract/test_extract.py
src/python/tests/unittests/test_controller/test_extract/test_dispatch.py
src/python/tests/unittests/test_ssh/test_sshcp_remote_address.py

src/python/ssh/sshcp.py

The copy() method still had a direct "{}@{}".format(user, host) that would produce "None@host" when user is None. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

coderabbitai bot reviewed Mar 24, 2026

View reviewed changes

src/python/ssh/sshcp.py Show resolved Hide resolved

Fix missed user@host format in Sshcp.copy() to use _remote_address()

38d2052

The copy() method still had a direct "{}@{}".format(user, host) that would produce "None@host" when user is None. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

nitrobass24 merged commit 34e6779 into develop Mar 24, 2026
12 checks passed

nitrobass24 deleted the fix/ruff-real-bugs branch March 24, 2026 01:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pre-existing code quality bugs (#289)#312

Fix pre-existing code quality bugs (#289)#312
nitrobass24 merged 2 commits intodevelopfrom
fix/ruff-real-bugs

nitrobass24 commented Mar 24, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Mar 24, 2026 •

edited

Loading

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

nitrobass24 commented Mar 24, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

nitrobass24 commented Mar 24, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 24, 2026 •

edited

Loading